Add implementation of mjd_ellipsoidFluid derivative #897
Conversation
Implements derivative of ellipsoid-based fluid forces with respect to velocities. Includes all component derivatives: - Added mass forces - Viscous torque and drag - Kutta lift - Magnus force The implementation adds the fluid force derivatives to qDeriv, following the pattern from the reference C implementation.
mujoco_warp/_src/derivative.py
Outdated
|
|
||
|
|
||
| @wp.kernel | ||
| def _qderiv_ellipsoid_fluid( |
There was a problem hiding this comment.
lets rename to _deriv_ellipsoid_fluid
mujoco_warp/_src/derivative.py
Outdated
|
|
||
|
|
||
| @wp.kernel | ||
| def _qderiv_ellipsoid_fluid( |
There was a problem hiding this comment.
lets make this a wp.func instead of a wp.kernel. additionally, it should have worldid and bodyid inputs.
mujoco_warp/_src/derivative.py
Outdated
| continue | ||
|
|
||
| size = geom_size[worldid % geom_size.shape[0], geomid] | ||
| semiaxes = _geom_semiaxes_deriv(size, geom_type[geomid]) |
There was a problem hiding this comment.
lets import _geom_semiaxes from passive.py. additionally let's change the name (in passive.py) to geom_semiaxes
mujoco_warp/_src/derivative.py
Outdated
| slender_drag_coef = geom_fluid[geomid, 2] | ||
| ang_drag_coef = geom_fluid[geomid, 3] | ||
|
|
||
| if density > 0.0 and magnus_coef != 0.0: |
There was a problem hiding this comment.
lets structure this with one if density > 0.0 check and checks for magnus_coef, kutta_coef and viscosity in the density check scope.
mujoco_warp/_src/derivative.py
Outdated
|
|
||
|
|
||
| @wp.func | ||
| def _pow2_deriv(val: float) -> float: |
There was a problem hiding this comment.
this function is unused, lets remove it
mujoco_warp/_src/derivative.py
Outdated
| lin_vel = wp.spatial_bottom(local_vels) | ||
| ang_vel = wp.spatial_top(local_vels) | ||
|
|
||
| virtual_lin_mom = wp.vec3( |
There was a problem hiding this comment.
fluid_density * wp.cw_mul(virtual_mass, lin_vel)
mujoco_warp/_src/derivative.py
Outdated
| fluid_density * virtual_mass[1] * lin_vel[1], | ||
| fluid_density * virtual_mass[2] * lin_vel[2] | ||
| ) | ||
| virtual_ang_mom = wp.vec3( |
There was a problem hiding this comment.
fluid_density * wp.cw_mul(virtual_inertia, ang_vel)
mujoco_warp/_src/derivative.py
Outdated
| ang_drag_coef * II[2] + slender_drag_coef * (I_max - II[2]) | ||
| ) | ||
|
|
||
| mom_visc = wp.vec3(x * mom_coef[0], y * mom_coef[1], z * mom_coef[2]) |
mujoco_warp/_src/derivative.py
Outdated
|
|
||
| mom_visc = wp.vec3(x * mom_coef[0], y * mom_coef[1], z * mom_coef[2]) | ||
| norm = wp.length(mom_visc) | ||
| density = fluid_density / wp.max(wp.static(1e-10), norm) |
There was a problem hiding this comment.
lets utilize MJ_MINVAL instead of wp.static(1e-10)
mujoco_warp/_src/derivative.py
Outdated
| norm = wp.length(mom_visc) | ||
| density = fluid_density / wp.max(wp.static(1e-10), norm) | ||
|
|
||
| mom_sq = wp.vec3( |
There was a problem hiding this comment.
-density * wp.cw_mul(wp.cw_mul(ang, mom_coef), mom_coef)
mujoco_warp/_src/derivative.py
Outdated
|
|
||
| proj_denom = aa * xx + bb * yy + cc * zz | ||
| proj_num = a * xx + b * yy + c * zz | ||
| dA_coef = wp.pi / wp.max(wp.static(1e-10), wp.sqrt(proj_num * proj_num * proj_num * proj_denom)) |
mujoco_warp/_src/derivative.py
Outdated
| proj_num = a * xx + b * yy + c * zz | ||
| dA_coef = wp.pi / wp.max(wp.static(1e-10), wp.sqrt(proj_num * proj_num * proj_num * proj_denom)) | ||
|
|
||
| A_proj = wp.pi * wp.sqrt(proj_denom / wp.max(wp.static(1e-10), proj_num)) |
mujoco_warp/_src/derivative.py
Outdated
| A_proj = wp.pi * wp.sqrt(proj_denom / wp.max(wp.static(1e-10), proj_num)) | ||
|
|
||
| norm = wp.sqrt(xx + yy + zz) | ||
| inv_norm = 1.0 / wp.max(wp.static(1e-10), norm) |
mujoco_warp/_src/derivative.py
Outdated
| xz, yz, zz + inner | ||
| ) | ||
|
|
||
| D = D * (-quad_coef * inv_norm) |
mujoco_warp/_src/derivative.py
Outdated
| proj_num = a * xx + b * yy + c * zz | ||
| norm2 = xx + yy + zz | ||
| df_denom = wp.pi * kutta_lift_coef * fluid_density / wp.max( | ||
| wp.static(1e-10), wp.sqrt(proj_denom * proj_num * norm2) |
mujoco_warp/_src/derivative.py
Outdated
| dfx_coef = yy * (a - b) + zz * (a - c) | ||
| dfy_coef = xx * (b - a) + zz * (b - c) | ||
| dfz_coef = xx * (c - a) + yy * (c - b) | ||
| proj_term = proj_num / wp.max(wp.static(1e-10), proj_denom) |
mujoco_warp/_src/derivative.py
Outdated
| dfy_coef = xx * (b - a) + zz * (b - c) | ||
| dfz_coef = xx * (c - a) + yy * (c - b) | ||
| proj_term = proj_num / wp.max(wp.static(1e-10), proj_denom) | ||
| cos_term = proj_num / wp.max(wp.static(1e-10), norm2) |
mujoco_warp/_src/derivative.py
Outdated
| proj_term = proj_num / wp.max(wp.static(1e-10), proj_denom) | ||
| cos_term = proj_num / wp.max(wp.static(1e-10), norm2) | ||
|
|
||
| D = wp.mat33( |
There was a problem hiding this comment.
a - a, b - b and c - c can be replaced with 0.0
mujoco_warp/_src/derivative.py
Outdated
| a - b, b - b, c - b, | ||
| a - c, b - c, c - c | ||
| ) | ||
| D = D * (wp.static(2.0) * proj_num) |
There was a problem hiding this comment.
*= and we can remove the parentheses
mujoco_warp/_src/derivative.py
Outdated
| ) | ||
| D = D * (wp.static(2.0) * proj_num) | ||
|
|
||
| inner_term = wp.vec3( |
There was a problem hiding this comment.
we can utilize wp.cw_mul here
mujoco_warp/_src/derivative.py
Outdated
| lin_vel_scaled = lin_vel * magnus_coef | ||
| ang_vel_scaled = ang_vel * magnus_coef | ||
|
|
||
| D_ang = _cross_deriv_a(ang_vel_scaled, lin_vel) |
There was a problem hiding this comment.
should lin_vel -> lin_vel_scaled?
mujoco_warp/_src/derivative.py
Outdated
| ang_vel_scaled = ang_vel * magnus_coef | ||
|
|
||
| D_ang = _cross_deriv_a(ang_vel_scaled, lin_vel) | ||
| D_lin = _cross_deriv_b(ang_vel, lin_vel_scaled) |
There was a problem hiding this comment.
should ang_vel -> ang_vel_scaled?
mujoco_warp/_src/derivative.py
Outdated
|
|
||
|
|
||
| @wp.func | ||
| def _cross_deriv_a(a: wp.vec3, b: wp.vec3) -> wp.mat33: |
There was a problem hiding this comment.
lets consider combining _cross_deriv_a and _cross_deriv_b into one function that returns 2 wp.mat33 since it looks like we always compute derivatives wrt to both cross product inputs
thowell
left a comment
thowell
left a comment
There was a problem hiding this comment.
@Aaryan-549 thank you for contributing this pr!
in addition to the comments, lets add a test. please let us know if you have any questions. thanks!
…J_MINVAL, restructure density checks, combine cross derivatives
|
Hey @thowell, really sorry for the delay. I have implemented all fixes that you asked for. Let me know If there are any other changes needed. |
|
@Aaryan-549 thank you for the contribution. It looks like there are some ruff linter issues, kernel analyzer issues, and unit test failures. You should be able to run these three tools locally to reproduce and fix the errors reported in CI. Please give that a go - once the issues are fixed we'll take another look at the PR.
|
|
Thanks, apologies for the noise. I'll run the tools locally and fix the errors shortly. |
|
I've run all three tools locally and fixed the issues. Ruff formatting and linting both pass. I've fixed 21 failing tests and now have 925/926 tests passing. The one remaining failure (test_collision19) is a pre-existing collision bug that was already failing before my commits. The kernel analyzer shows 3 style warnings in my code - two missing "# In:" comments and one parameter ordering convention issue. These don't break anything but I can fix them if you'd like, or address them in a follow-up after review. |
|
@Aaryan-549 thank you for the update! all of the checks should pass including tests, formatting, and the linter. thanks! |
|
we should also add a test to derivative_test.py that compares the result |
|
@Aaryan-549 Please add forward test for the implicitfast integration for ellispoid fluid model as well. You can refer to #1022 |
|
Just did! |
There was a problem hiding this comment.
lets remove the changes to this file since they are unrelated to the derivative feature and please submit a separate pr. thanks!
|
@SUZ-tsinghua would you be open to contributing a review to this pr? |
mujoco_warp/_src/io.py
Outdated
| mujoco.mjtIntegrator.mjINT_IMPLICIT, | ||
| ): | ||
| raise NotImplementedError(f"Implicit integrators and fluid model not implemented.") | ||
| # Removed check: Implicit integrators and fluid model are now supported |
There was a problem hiding this comment.
since the implicit integrator is not added in this pr we should keep this check
mujoco_warp/_src/io_test.py
Outdated
| </mujoco> | ||
| """ | ||
| ) | ||
| """Tests that implicit integrator with fluid model is now supported.""" |
There was a problem hiding this comment.
we can revert this change since the implicit integrator is not implemented in this pr
mujoco_warp/_src/forward_test.py
Outdated
| mjw.step(m, d) | ||
| self.assertGreater(d.time.numpy()[0], 0.0) | ||
|
|
||
| @parameterized.product( |
There was a problem hiding this comment.
since the implicit integrator is not implemented in this pr we can remove this test
mujoco_warp/_src/derivative_test.py
Outdated
| _assert_eq(mjw_out, mj_out, "qM - dt * qDeriv") | ||
|
|
||
| @parameterized.parameters( | ||
| (mujoco.mjtJacobian.mjJAC_DENSE, "ellipsoid"), |
There was a problem hiding this comment.
no need to parameterized 'fluidshape' since both cases are ellipsoid
mujoco_warp/_src/derivative_test.py
Outdated
| <site name="site2" pos="0 0 1"/> | ||
| </body> | ||
| </worldbody> | ||
| <tendon> |
There was a problem hiding this comment.
do we need tendons for this test?
mujoco_warp/_src/derivative_test.py
Outdated
| <site site="site2"/> | ||
| </spatial> | ||
| </tendon> | ||
| <actuator> |
There was a problem hiding this comment.
do we need actuators for this test?
|
Hi @thowell , I'd happy to review this pr. @Aaryan-549 For the derivative test, there is no need to include tendon and actuator. You can just copy my test. |
|
Thanks @thowell! I've addressed all the latest feedback in the recent push:
Checks are passing. Ready for final review! |
There was a problem hiding this comment.
please revert all changes to this file
There was a problem hiding this comment.
we shouldn't need any changes to this file
There was a problem hiding this comment.
we shouldn't need any changes to this file
There was a problem hiding this comment.
we shouldn't need any changes to this file
vector-one
force-pushed
the
feature/add-mjd-ellipsoidfluid-implementation
branch
2 times, most recently
from
e618379 to
7ecd897
Compare
…alues need debugging)
|
Hey @thowell , I am really sorry for taking a whole week to reply back but I've been working on implementing the The Problem: What I've Verified/Fixed:
Current State: Side Note: Any pointers on what might be causing the mismatch would be really appreciated. Happy to hop on a call if that would be easier. Thanks! |
|
Hi @Aaryan-549 , on my side, I only encountered 3 failed io tests on my branch. I have fixed these errors by merging the original branch into my branch. |
|
Hey @SUZ-tsinghua even I am facing 3 failed tests on my end(for my code). Do you have any idea of what could be causing these?(we only need to take care of 2 as 1 of the test was failing in main too, so I don't think we caused it). |
4 participants
Fixes #750
Implements derivative of ellipsoid-based fluid forces with respect to velocities. Includes all component derivatives:
The implementation adds the fluid force derivatives to qDeriv, following the pattern from the reference C implementation.